-
Notifications
You must be signed in to change notification settings - Fork 27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: unresponsive ledger device #474
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## dev #474 +/- ##
========================================
- Coverage 9.34% 9.34% -0.01%
========================================
Files 112 112
Lines 5166 5167 +1
Branches 698 698
========================================
Hits 483 483
- Misses 4020 4021 +1
Partials 663 663 ☔ View full report in Codecov by Sentry. |
// This ensures that a previously closed hardware wallet will not be loaded again | ||
// unless the device is connected | ||
const accessData = JSON.parse(accessDataRaw); | ||
if ((accessData?.walletFlags & 0x02) > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0x02
is the hardware wallet flag (ref).
We can expose WALLET_FLAGS
from the lib on the next version and import it here but I wish to release this fix asap so I hardcoded the value here for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you create the issue for that, please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
package.json
Outdated
"@hathor/wallet-lib": "1.0.2", | ||
"@ledgerhq/hw-transport-node-hid": "6.27.1", | ||
"@hathor/wallet-lib": "1.0.4", | ||
"@ledgerhq/hw-transport-node-hid": "^6.28", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: Could we use a pinned version for this dependency? This has been the default since #422
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
860e40f
to
416a4db
Compare
console.log('Error communicating with device'); | ||
console.log(error); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: Maybe switch these error messages to console.error
instead?
// This ensures that a previously closed hardware wallet will not be loaded again | ||
// unless the device is connected | ||
const accessData = JSON.parse(accessDataRaw); | ||
if ((accessData?.walletFlags & 0x02) > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you create the issue for that, please?
Summary
Fix the issues described in #473
Acceptance Criteria
Security Checklist